Fix rewind deadlock when Task.all had multiple failed siblings (#299)#300
Open
YunchuWang wants to merge 1 commit into
Open
Fix rewind deadlock when Task.all had multiple failed siblings (#299)#300YunchuWang wants to merge 1 commit into
YunchuWang wants to merge 1 commit into
Conversation
After a server-side (Azure Storage / DurableTask.Core) rewind, a nested orchestration whose Task.all fan-out contained more than one failed activity would deadlock: only one previously-failed sibling was re-dispatched and the other became a permanently-unresolved pending task, so the child's Task.all never resolved and the parent/root never completed (30s timeout). Root cause: the rewind removes a failed activity's TaskScheduled only when a TaskFailed event exists for it. Task.all fails fast, so after the first sibling failure the child completes-failed and later siblings' TaskFailed events are never committed. Their TaskScheduled is therefore left BARE (no terminal). On replay handleTaskScheduled deletes the pending action assuming a completion will arrive, but after a rewind none ever does -> the task is orphaned. Fix (worker executor): - Detect a rewind revival from the NEW events (Azure Storage wakes the instance with a GenericEvent; DTS uses ExecutionRewound). Gating on new events keeps reconciliation scoped to the revival work item so ordinary post-rewind replays never re-dispatch genuinely in-flight tasks. - Archive each consumed scheduleTask action during replay; at end of a revival replay, re-dispatch every orphaned activity (pending task whose scheduleTask action was consumed by a bare TaskScheduled but has no live pending action). General for N failed siblings. - Make a duplicate TaskScheduled for an already-handled id idempotent, since the re-dispatch causes DurableTask.Core (no dedup) to append a second TaskScheduled with the same id. Adds a deterministic executor-level regression test that models the classic Azure Storage backend (append-only TaskScheduled) and is red without the fix / green with it, plus a no-regression test asserting an in-flight bare TaskScheduled during ordinary replay is not re-dispatched. Closes #299 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a worker-side replay deadlock after server-side rewinds (Azure Storage / DurableTask.Core) when a nested orchestration’s Task.all fan-out had multiple failed activity siblings, leaving one or more siblings as bare TaskScheduled events with no terminal completion and causing the child (and therefore parent) orchestration to hang.
Changes:
- Adds rewind-revival reconciliation that re-dispatches “orphaned” activity schedule actions whose
TaskScheduledwas consumed during replay but never receives a terminal event post-rewind. - Makes duplicate
TaskScheduledevents for the same task ID idempotent during replay to tolerate Azure Storage’s append-only scheduling behavior. - Adds deterministic executor-level regression tests modeling the Azure Storage backend rewind shape and duplicate
TaskScheduledbehavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/durabletask-js/src/worker/orchestration-executor.ts | Rewind-revival detection + orphaned activity re-dispatch reconciliation; idempotent handling for duplicate TaskScheduled. |
| packages/durabletask-js/src/worker/runtime-orchestration-context.ts | Adds executor bookkeeping to remember consumed activity actions and handled taskScheduled IDs. |
| packages/durabletask-js/test/rewind-multi-sibling-deadlock.spec.ts | New executor-level regression suite reproducing and guarding the multi-failed-sibling rewind deadlock scenario. |
Comment on lines
+162
to
+164
| private isRewindRevival(newEvents: pb.HistoryEvent[]): boolean { | ||
| return newEvents.some((e) => e.hasExecutionrewound() || e.hasGenericevent()); | ||
| } |
Comment on lines
+128
to
+134
| const impl = activityImpls[name]; | ||
| try { | ||
| const output = impl ? impl(input) : undefined; | ||
| committed.push(newTaskCompletedEvent(id, output !== undefined ? JSON.stringify(output) : undefined)); | ||
| } catch (err) { | ||
| committed.push(newTaskFailedEvent(id, err as Error)); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a rewind deadlock where a nested orchestration whose
Task.allfan-out contained more than one failed activity never completes after a server-side (Azure Storage / DurableTask.Core) rewind. The worker re-dispatched only one previously-failed sibling and left the other as a permanently-unresolved pending task, so the child'sTask.allnever resolved, noSubOrchestrationInstanceCompletedwas emitted, and the parent/root hung until the 30s timeout.Closes #299Root cause (verified)
On the Azure Storage backend, rewind is computed server-side by DurableTask.Core — this is a plain replay of a backend-rewritten history, not the
EXECUTIONREWOUND/buildRewindResultpath from #296 (0ExecutionRewoundevents observed).The rewind (
DurableTask.AzureStorageRewindHistoryAsync/DurableTask.CoreProcessRewindOrchestrationDecision) removes/morphs a failed activity'sTaskScheduledonly when aTaskFailedevent exists for it. BecauseTask.allfails fast, the child completes-failed after the first sibling failure, so later siblings'TaskFailedevents are never committed (late completion for a terminal instance is dropped). After rewind:TaskFailedpresent): itsTaskScheduledis morphed away → its pending action survives replay → re-dispatched cleanly. ✅TaskFailedmissing): itsTaskScheduledis left bare (no terminal) →handleTaskScheduleddeletes its pending action assuming a completion will arrive, but after a rewind none ever does → orphaned pending task →Task.alldeadlocks. ❌Observed worker trace (single child instance):
Waiting for 2 task(s) … Returning 1 action(s)(one sibling re-dispatched, runs, succeeds) thenWaiting for 1 task(s) … Returning 0 action(s)= deadlock.Fix (worker executor)
GenericEvent(RewindTaskOrchestrationAsyncsendsnew GenericEvent(-1, reason)); the DTS jump-start path usesExecutionRewound. Gating on new events scopes reconciliation to the revival work item, so ordinary post-rewind replays (whose new events are normal completions) never re-dispatch genuinely in-flight tasks.scheduleTaskaction. At the end of a revival replay, for every still-pending task whosescheduleTaskaction was consumed by a bareTaskScheduledbut has no live pending action, re-add the archived action. General for N failed siblings. Sub-orchestrations are intentionally excluded (their retainedSubOrchestrationInstanceCreatedis revived separately by the backend).TaskScheduledidempotent.DurableTask.CoreProcessScheduleTaskDecisionalways appends a freshTaskScheduledper action (no dedup), so re-dispatching the bare orphan produces a secondTaskScheduledwith the same id. A repeat for an already-handled id is now treated as idempotent instead of raisingNonDeterminismError.57 lines of source across 2 files (
orchestration-executor.ts,runtime-orchestration-context.ts).Test evidence
New deterministic executor-level regression test
test/rewind-multi-sibling-deadlock.spec.tsdrives the executor through episodes modelling the classic Azure Storage backend (append-onlyTaskScheduled, revivalGenericEventtrigger):Task.allsiblings after a rewind and completes — red without the fix (deadlock), green with it.TaskScheduledduring ordinary (non-rewind) replay — no-regression guard (passes onmainand with the fix).Red/green proof (identical test code):
Sister-SDK parity (corroboration)
The same defect exists in durabletask-python (independently verified against
microsoft/durabletask-python@e7b5f15), which confirms this is a core, cross-SDK replay-contract issue rather than a JS-specific quirk:taskScheduledhandler (worker.py~L2196-2205) pops the pending action on a bareTaskScheduledand raises a non-determinism error if the action is already gone —action = ctx._pending_actions.pop(task_id, None); if not action: raise _get_non_determinism_error(...)— while only reading (.get) the pending task. So a rewound-away sibling leaves an orphaned pending task, exactly as in JS.get_actions()(worker.py~L1549) returnslist(self._pending_actions.values()), so a popped orphan is never re-dispatched → identicalTask.alldeadlock after a multi-failed-sibling rewind._build_rewind_result) only short-circuits the DTS jump-start path (anexecutionRewoundevent in the new events). A classic Azure Storage server-side rewind (noexecutionRewound) falls through to normal replay — the same path that deadlocks here — and there is no_consumed/ rewind-revival re-dispatch and no idempotent duplicate-TaskScheduledguard. The durabletask-python functions-support PR Andystaples/add functions support durabletask-python#155 only added agenericEventignore, not re-dispatch, so the deadlock remains unfixed there.This independently corroborates two things about this PR:
if not action: raise NonDeterminismon a repeatTaskScheduledis exactly what a naive re-dispatch would trip — because re-dispatching the orphan makes the backend append a secondTaskScheduledfor the same id. Re-dispatch without the idempotency guard would convert the deadlock into aNonDeterminismError, which is why this PR pairs re-dispatch with idempotent duplicate-TaskScheduledhandling.A durabletask-python parity fix is therefore warranted and should be tracked separately.
E2E status on real Azure Storage (verified)
Run on a live Azure Storage backend (fix logic ported onto the #282 gRPC-worker core; fix anchors byte-identical;
callEntities=false, single target framework to force one deterministic instance):TaskScheduledassumption is CLOSED — zeroNonDeterminismErroracrossnumFailures1 and 2.Waiting for 2 → Returning 2(both failed activity siblings re-dispatched) and completes, whereas the un-fixed baseline logsReturning 1and deadlocks — the decisive proof this fix is real and necessary.This fix is necessary but not sufficient to turn
RewindOrchestratorTests.RewindFailedOrchestration_ShouldSucceed(1|2)green, because that E2E also exercises a distinct, deeper gap: the rootTask.allfails fast on the first sub-orchestration failure, so the second failing sub-orch's failure is dropped (late completion for a now-terminal parent). Server-side rewind therefore signals only the committed-failed subtree; the other failing subtree is re-driven with no rewind signal, so its grandchild hits the identical #299 orphan that the (correctly gate-guarded) reconcile cannot repair without a signal. Which subtree is dropped is a race. That unsignaled-sibling gap is tracked separately in #301.Gates
npm run build— clean (all workspaces)npm run lint— cleannpm run test:unit— 1283 passed, 0 failed (1122 core incl. the new spec and the Add rewind support #296rewind-e2esuite, 106 azuremanaged, 55 export-history)Notes / honesty
DurableTask.Core/DurableTask.AzureStoragerewind source, the failing-run worker log, and the issue Rewind of nested orchestration deadlocks when a Task.all had multiple failed activities (only one failed sibling re-dispatched on replay) #299 A/B evidence.TaskScheduledbehavior the idempotent guard handles. The full extension E2E was additionally run live on Azure Storage (see E2E status above): on the path DTFx actually rewinds it confirms the Rewind of nested orchestration deadlocks when a Task.all had multiple failed activities (only one failed sibling re-dispatched on replay) #299 fix — the signaled grandchild logsWaiting for 2 → Returning 2(both failed siblings re-dispatched) and completes, versus the NOFIX baseline'sReturning 1deadlock — with zeroNonDeterminismErroracross every run, so the idempotent handled-id guard holds on the real backend. However, clean single-target-framework runs ofRewindFailedOrchestration_ShouldSucceed(1|2)still fail atRewindOrchestratorTests.cs:50for a distinct, deeper reason — an unsignaled re-driven sibling subtree, tracked in Rewind after a multi-failed-sibling WhenAll deadlocks the UNSIGNALED sibling subtree (emit-once replay can't reconcile a re-driven orphan with no rewind signal) #301 — and an earlier apparent pass was a multi-TFM (net8.0;net10.0) concurrency artifact, not a reliable green. Fix rewind deadlock when Task.all had multiple failed siblings (#299) #300 stays correctly scoped to the Rewind of nested orchestration deadlocks when a Task.all had multiple failed activities (only one failed sibling re-dispatched on replay) #299 signaled-path fix.Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com